Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 113ca0d The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
TL;DR — Adds the Key changes
Summary ｜ 2 files ｜ 1 commit ｜ base:
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Small, focused change — adds app.id to the active span in the two runApiKeyAuthHandler paths. The naming convention (app.id) matches the existing pattern (tenant.id, agent.id, project.id). One minor inconsistency worth addressing.
| if (reqData.appId && attempt.authResult) { | ||
| trace.getActiveSpan()?.setAttribute('app.id', reqData.appId); | ||
| } |
There was a problem hiding this comment.
The dev-environment guard checks attempt.authResult in addition to reqData.appId, but the production path (line 764) only checks reqData.appId. In the dev fallback branch (line 702–709), when appId is present but auth fails, the span won't get the attribute — even though the request is clearly associated with that app.
Since reqData.appId is the only thing that matters for attributing the span, consider dropping the attempt.authResult check to match the production path:
| if (reqData.appId && attempt.authResult) { | |
| trace.getActiveSpan()?.setAttribute('app.id', reqData.appId); | |
| } | |
| if (reqData.appId) { | |
| trace.getActiveSpan()?.setAttribute('app.id', reqData.appId); | |
| } |
| "@inkeep/agents-api": patch | ||
| --- | ||
|
|
||
| adding app.id to span attributes |
There was a problem hiding this comment.
Nit: changelog messages should start with a capitalized action verb per the repo conventions (e.g. "Add app.id to span attributes").
| adding app.id to span attributes | |
| Add `app.id` to span attributes |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This PR adds the app.id attribute to OpenTelemetry spans during run API authentication, enabling trace filtering and correlation by application. The implementation is clean, correct, and follows established patterns.
🟢 What's Good
- Correct pattern usage: Uses
trace.getActiveSpan()?.setAttribute()consistent with existing telemetry code - Proper naming: Follows the
{resource}.idconvention (tenant.id,project.id,agent.id) - Low cardinality: App IDs are admin-created credentials, not user-generated, so this won't cause cardinality explosion
- No PII exposure: The attribute is an opaque identifier
- Intentional guard logic: The different conditions between dev (
&& attempt.authResult) and prod paths are correct — prod throws earlier on null auth, while dev should only attribute spans when real auth succeeded (not dev fallback)
💭 Consider (C) 💭
Inline Comments:
- 💭 Consider:
runAuth.ts:713— Optionally addAPP_IDto SPAN_KEYS constants for consistency (though mixed patterns exist in codebase)
✅ APPROVE
Summary: Clean, focused observability improvement. The code is correct, follows established patterns, and enables useful trace filtering by app. Ship it! 🚀
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-sre |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
2 | 0 | 0 | 0 | 1 | 0 | 1 |
| Total | 2 | 0 | 0 | 0 | 1 | 0 | 1 |
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
runAuth.ts:712,764 |
Inconsistent guard conditions between dev/prod paths | Analyzed and found to be intentional — prod path throws before line 764 if authResult is null, while dev path needs the extra check to avoid attributing spans when using fallback dev context |
| } | ||
|
|
||
| if (reqData.appId && attempt.authResult) { | ||
| trace.getActiveSpan()?.setAttribute('app.id', reqData.appId); |
There was a problem hiding this comment.
💭 Consider: Add app.id to SPAN_KEYS constants
Issue: The new 'app.id' attribute is defined as a string literal, while the codebase has an established pattern of centralizing span attribute keys in SPAN_KEYS in packages/agents-core/src/constants/otel-attributes.ts.
Why: Centralizing attribute names helps maintain a consistent telemetry schema and makes it easier to audit what attributes are being emitted. However, I note the codebase has some mixed patterns (e.g., chat.ts also uses inline strings like 'user.type'), so this is a suggestion rather than a requirement.
Fix: Optionally add to otel-attributes.ts:
APP_ID: 'app.id',Then import and use SPAN_KEYS.APP_ID here.
Refs:
- otel-attributes.ts — existing SPAN_KEYS definitions
1 participant
No description provided.